-
Notifications
You must be signed in to change notification settings - Fork 5
Add tag subcommand
#96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ianthomas23
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The git tag -l v* definitely won't work in the wasm build. so I'll need to support that soon.
src/subcommand/tag_subcommand.cpp
Outdated
| { | ||
| return; | ||
| } | ||
| if (pos < message.length() && pos + 1 < message.length()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If pos + 1 < message.length() is true then pos < message.length() must also be so it is not necessary to check both.
| } | ||
|
|
||
| // Tag listing: Print individual message lines | ||
| void print_list_lines(const std::string& message, int num_lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function looks quite complicated for a relatively simple task. Maybe a better approach would be to first split the string at newline characters into a vector of strings and walk that vector. There is already a split-string-at-newlines function at
git2cpp/src/utils/terminal_pager.cpp
Line 199 in 4bd9b8e
| void terminal_pager::split_input_at_newlines(std::string_view str) |
which could be pulled out of that file and reused.
But that could be in a later separate PR.
src/subcommand/tag_subcommand.cpp
Outdated
| std::string pattern = m_tag_name.empty() ? "*" : m_tag_name; | ||
| auto tag_names = repo.tag_list_match(pattern); | ||
|
|
||
| for (size_t i = 0; i < tag_names.size(); i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could avoid the loop variable here by using a range-based for loop like this instead:
for (const auto& tag_name: tag_names)
{
each_tag(repo, tag_name, m_num_lines);
}
src/wrapper/repository_wrapper.cpp
Outdated
| throw_if_error(git_tag_list_match(&tag_names, pattern.c_str(), *this)); | ||
|
|
||
| std::vector<std::string> result; | ||
| for (size_t i = 0; i < tag_names.count; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a range-based for loop here too.
src/subcommand/tag_subcommand.cpp
Outdated
| throw git_exception("tag '" + m_tag_name + "' already exists", git2cpp_error_code::FILESYSTEM_ERROR); | ||
| } | ||
| throw git_exception("Unable to create annotated tag", error); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has a lot in common with create_lightweight_tag, maybe it would be worth extracting the common parts in small functions and have the create_XXX_tag be simple "drivers".
src/subcommand/tag_subcommand.cpp
Outdated
| throw git_exception("Tag name required", git2cpp_error_code::GENERIC_ERROR); | ||
| } | ||
|
|
||
| if (m_message.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is already tested before calling create_tag, so this check can be removed.
Fix #64
Add
tagsubcommand.